Skip to content

[CI] Disable C++20 module build to fix CI failures#91

Closed
Luohaothu wants to merge 7 commits intomasterfrom
ci/disable-module-build
Closed

[CI] Disable C++20 module build to fix CI failures#91
Luohaothu wants to merge 7 commits intomasterfrom
ci/disable-module-build

Conversation

@Luohaothu
Copy link
Contributor

@Luohaothu Luohaothu commented Feb 4, 2026

Problem

The current CI configuration with OPFLOW_ENABLE_MODULE=ON is causing build failures:

error: invalid conversion from 'int' to 'MPI_Comm' {aka 'ompi_communicator_t*'} [-fpermissive]

These errors occur in multiple Solver classes when building with modules enabled and MPI support. This is blocking all PRs, including documentation-only changes like PR #88.

Solution

This PR temporarily disables module builds in CI by setting -DOPFLOW_ENABLE_MODULE=OFF.

Changes

  • Set OPFLOW_ENABLE_MODULE=OFF in .github/workflows/Build.yml
  • This allows CI to pass while module support issues are fixed separately

Follow-up Work

Module support should be fixed and re-enabled in a dedicated PR that includes:

  • Fix MPI_Comm type conversion issues in Solver classes
  • Test module builds with both MPI=ON and MPI=OFF
  • Verify compatibility across GCC 15 and Clang 21
  • Add proper type guards for MPI types when modules are enabled

Test Plan

Related Issues

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Enhanced compiler warning configuration for improved code quality across MSVC, GNU, and Clang compilers.
    • Updated build system to enforce stricter compilation standards based on module enablement settings.
    • Adjusted build target scope and visibility settings for better build consistency.

The current CI configuration with OPFLOW_ENABLE_MODULE=ON is causing
build failures due to MPI_Comm type conversion issues in module builds.
This is blocking all PRs including documentation-only changes.

This commit disables module builds in CI as a temporary measure to
unblock PRs. Module support should be fixed and re-enabled in a
dedicated PR that includes:
- Fixing MPI_Comm type issues in Solver classes
- Testing module builds with both MPI ON and OFF
- Verifying compatibility across GCC 15 and Clang 21

Related issue: Module builds fail with -fpermissive errors

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@gemini-code-assist
Copy link

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The changes modify the CMake build configuration to conditionally manage compiler visibility and warning flags. A macro definition's visibility changes from PUBLIC to PRIVATE, compiler-specific warning options are introduced with conditional scope based on module enablement, and a C++20 comment updates to C++26.

Changes

Cohort / File(s) Summary
Build Configuration
src/CMakeLists.txt
Modified target_compile_definitions scope to PRIVATE, introduced OPFLOW_TARGET_SCOPE logic conditional on OPFLOW_ENABLE_MODULE, added compiler-specific strict warning flags (MSVC /W4 /WX, GNU/Clang -Wall -Wextra -Werror), updated version comment from C++20 to C++26.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 The scope now hops from PUBLIC bright,
To PRIVATE whispers, perfectly right,
With warnings strict from every compiler's might,
C++26 sparkles, a glorious sight!
Hopping onward with cleaner delight! ✨🔨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '[CI] Disable C++20 module build to fix CI failures' accurately describes the main change: disabling OPFLOW_ENABLE_MODULE in the CI workflow configuration to resolve build failures.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ci/disable-module-build

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66008bb5ff

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Luohaothu and others added 6 commits February 4, 2026 23:33
- Remove redundant -B build flag (working-directory is already ./build)
- Add .. as source directory argument
- Change cmake --build from 'build' to '.' to use current directory

This fixes: CMake Error: The source directory does not appear to contain CMakeLists.txt

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
When OPFLOW_ENABLE_MODULE=OFF, the opflow target is an INTERFACE library.
CMake requires that INTERFACE libraries only use INTERFACE keyword when
setting properties like target_include_directories, target_link_directories,
and target_link_libraries.

This fixes:
- target_include_directories may only set INTERFACE properties on INTERFACE targets
- target_link_directories may only set INTERFACE properties on INTERFACE targets
- INTERFACE library can only be used with the INTERFACE keyword

Changed all PUBLIC to INTERFACE in external/CMakeLists.txt for TecIO setup.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Import CMake configuration from fix-module branch that correctly handles
both module (PUBLIC scope) and header-only (INTERFACE scope) builds.

Key changes:
- Introduce OPFLOW_TARGET_SCOPE variable (PUBLIC for module, INTERFACE for header-only)
- Use ${OPFLOW_TARGET_SCOPE} throughout instead of hardcoded PUBLIC/INTERFACE
- Update target_sources with proper BASE_DIRS for header-only build
- Change to C++26 in header-only mode (was C++20)
- Add -Wall -Werror compiler flags with -Wno-psabi
- Fix all target_link_libraries calls to use correct scope

This resolves:
- target_sources/target_include_directories/target_link_libraries
  INTERFACE library property errors
- Allows building with OPFLOW_ENABLE_MODULE=OFF

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Define OPFLOW_TARGET_SCOPE in main CMakeLists.txt right after OPFLOW_ENABLE_MODULE option
- Remove duplicate definition from src/CMakeLists.txt
- Ensures variable is available before use in src/ and external/

This fixes: target_include_directories called with invalid arguments

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@Luohaothu Luohaothu closed this Feb 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments